feat[ServletAdapter]: Ability remove context path when get method name by long76 · Pull Request #11730 · grpc/grpc-java

long76 · 2024-12-09T14:49:25Z

Complex application may have many war-s and do not allow monopolize root context path(/). Add ability to remove context path(usual it's war name) from application server use @WebInitParam or just override getInitParameter for parameter GrpcServlet.REMOVE_CONTEXT_PATH

requestURI = contextPath + servletPath + pathInfo

https://stackoverflow.com/a/42706412

Refs: grpc#5066

linux-foundation-easycla · 2024-12-09T14:49:30Z

The committers listed above are authorized under a signed CLA.

✅ login: long76 (2495095, 21e48be, 2f4aea5, 33ed4c6, bb7a6d4, ed0fdbd, 27a99bc, 9ffaeb7, 2c1c2f1, 4bb9d65)

panchenko · 2024-12-09T15:09:20Z

Could you please describe the problem you are trying to solve?

long76 · 2024-12-09T15:21:38Z

Could you please describe the problem you are trying to solve?

I use Wildfly 24 and need use non root context(/) for GRPC. I can use custom path for GRPC server? On GRPC clients like Insomnia and Postman i can't put localhost:8080/grpc-war in url. This MR allow use custom context for application servers like WildFly. If i will use HAProxy or something else for proxy i can get access for GRPC server on Wildfly 24 on GrpcServlet.

panchenko · 2024-12-09T22:02:29Z

I can use custom path for GRPC server?

No.

Insomnia and Postman

They are not real clients, but UI tools to send arbitrary requests to server for development purposes.

ejona86 · 2024-12-26T19:31:47Z

Yeah, there's not going to be clients that work with this. It sounds like you would get testing infrastructure running, but once you try to make real clients you're going to have problems.

long76 · 2025-01-14T03:58:29Z

@ejona86 local fork works great on Wildfly 24, my patch do not break backward compatibility

ejona86 · 2025-01-14T16:17:32Z

The server is irrelevant. We were saying the clients won't support this as gRPC doesn't support this.

long76 · 2025-01-14T16:26:08Z

The server is irrelevant. We were saying the clients won't support this as gRPC doesn't support this.

This will work with proxy. Nginx > Wildfly > GRPCServlet works. I do not see limitations for support this for GRPCServlet.

panchenko · 2025-01-14T16:31:28Z

@long76 Do you mean ngx_http_grpc_module ?
It does not support changing request path.

long76 · 2025-01-14T16:47:16Z

@long76 Do you mean ngx_http_grpc_module ?
It does not support changing request path.

😏Just try this config for nginx:

server {
        server_name mygrpc-sever.com;
        listen 3000 http2;

        location / {
                rewrite ^(/.*)$ /grpc$1 break;
                grpc_pass grpcs://ip_or_domain_wildfly:443;
        }
}

P.S. i don't know it's bug or feature nginx but i glad this flexibility)

ejona86

Doesn't REMOVE_CONTEXT_PATH mean that you must put the servlet under its own "folder" because it will need a wildcard for the full service/method. So where your app may be /app1, you'd then have to do something like /app1/grpc/* for the servlet so that the requested path is /app1/grpc/helloworld.Greeter/SayHello?

ejona86 · 2025-01-14T17:28:30Z

   * <p>Do not modify {@code req} and {@code resp} before or after calling this method. However,
   * calling {@code resp.setBufferSize()} before invocation is allowed.
   */
-  public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {


Deleting this breaks compatibility. If you have to modify the helloworld example (hard-coding a string no less!), then that clearly is API-breaking.

i agree that hard-coded string is bad but for now i dont found better solution, i need more time or you can suggest smth

I don't follow the difficulty.

public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException { doPost(req.getRequestURI().substring(1), req, resp); }

ejona86 · 2025-01-14T17:29:14Z

  }

+  private String getMethod(HttpServletRequest req) {
+    Boolean removeContextPath = Boolean.parseBoolean(getInitParameter(REMOVE_CONTEXT_PATH));


This shouldn't be done per-request. We should do this in init instead, right?

i agree, fix it soon

ejona86 · 2025-01-14T17:37:36Z

  }

+  private String getMethod(HttpServletRequest req) {
+    Boolean removeContextPath = Boolean.parseBoolean(getInitParameter(REMOVE_CONTEXT_PATH));


s/Boolean/boolean/

ejona86 · 2025-01-14T17:43:16Z

+    String method = req.getRequestURI();
+    if (removeContextPath) {
+      // remove context path used in application server
+      method = method.substring(req.getContextPath().length());


Isn't this mostly the same as req.getPathInfo()? I wouldn't expect to need to do any string manipulation.

no, at first I thought the same thing. only getContextPath() needed. see https://coderanch.com/t/610432/certification/getContextPath-getServletPath-getPathInfo

I see, you are making it relative to the deployment.

long76 · 2025-01-14T18:52:22Z

Doesn't REMOVE_CONTEXT_PATH mean that you must put the servlet under its own "folder" because it will need a wildcard for the full service/method. So where your app may be /app1, you'd then have to do something like /app1/grpc/* for the servlet so that the requested path is /app1/grpc/helloworld.Greeter/SayHello?

in our case we create dedicated grpc.war for it with root path /grpc and for everything to work need just remove contextPath

long76 · 2025-01-14T18:52:51Z

new pull #11825

panchenko · 2025-01-14T20:25:35Z

@long76 Have you considered name-based virtual hosting? and then deploy this code as ROOT.war in the "mygrpc" host.
So it can work even without nginx.

long76 · 2025-01-15T18:19:26Z

@long76 Have you considered name-based virtual hosting? and then deploy this code as ROOT.war in the "mygrpc" host. So it can work even without nginx.

yeah, but we need our business logic from our app. we use ejb for access data from other wars.

panchenko · 2025-01-15T20:46:00Z

@long76 There is also an "enterprise" way - you could use a servlet filter.

long76 · 2025-01-15T21:26:36Z

@long76 There is also an "enterprise" way - you could use a servlet filter.

yeah but it doesn't work) it's was first that i try) maybe i make mistake somewhere

panchenko · 2025-01-17T11:10:32Z

@long76 it seems like in your gRPC servlet class in doPost you can create a wrapper and pass it further

new HttpServletRequestWrapper(req) {
    @Override
    public String getRequestURI() {
        return super.getRequestURI().replaceFirst("^/grpc", "");
    }
};

As your setup does not quite match the current state of gRPC- not clear if the related changes should be in the library.

long76 · 2025-01-17T11:50:01Z

@long76 it seems like in your gRPC servlet class in doPost you can create a wrapper and pass it further
new HttpServletRequestWrapper(req) {
    @Override
    public String getRequestURI() {
        return super.getRequestURI().replaceFirst("^/grpc", "");
    }
};
As your setup does not quite match the current state of gRPC- not clear if the related changes should be in the library.

Yeah, but no)

'doPost(HttpServletRequest, HttpServletResponse)' cannot override 'doPost(HttpServletRequest, HttpServletResponse)' in 'io. grpc. servlet. GrpcServlet'; overridden method is final

Yes my changes does not match grpc library but it's very useful for application server users and finally it's easy realize in library)

panchenko · 2025-01-17T12:55:18Z

@long76 you can copy GrpcServlet into your code base, that's a small class )

long76 · 2025-01-17T13:22:28Z

@long76 you can copy GrpcServlet into your code base, that's a small class )

Copy paste 😒 i better use our release with patch)

feat[ServletAdapter]: Ability remove context path when get method name

2495095

Refs: grpc#5066

fix: getMethod must be not static

4bb9d65

long76 marked this pull request as draft December 9, 2024 15:02

fix: wrong realisation

2f4aea5

long76 added 7 commits December 9, 2024 18:24

fix[ServletAdapter]: java doc

33ed4c6

fix: code style

9ffaeb7

fix[ServletServerBuilderTest]: compile test

bb7a6d4

fix[bb7a6d4]: var name

2c1c2f1

fix[HelloWorldServlet]: example

27a99bc

fix[ServletServerBuilderTest]: use verify

21e48be

fix: use hardcoded method

ed0fdbd

long76 marked this pull request as ready for review December 11, 2024 17:20

ejona86 closed this Jan 13, 2025

ejona86 reviewed Jan 14, 2025

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2025

Conversation

long76 commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panchenko commented Dec 9, 2024

Uh oh!

long76 commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panchenko commented Dec 9, 2024

Uh oh!

ejona86 commented Dec 26, 2024

Uh oh!

long76 commented Jan 14, 2025

Uh oh!

ejona86 commented Jan 14, 2025

Uh oh!

long76 commented Jan 14, 2025

Uh oh!

panchenko commented Jan 14, 2025

Uh oh!

long76 commented Jan 14, 2025

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

long76 commented Jan 14, 2025

Uh oh!

long76 commented Jan 14, 2025

Uh oh!

panchenko commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

long76 commented Jan 15, 2025

Uh oh!

panchenko commented Jan 15, 2025

Uh oh!

long76 commented Jan 15, 2025

Uh oh!

panchenko commented Jan 17, 2025

Uh oh!

long76 commented Jan 17, 2025

Uh oh!

panchenko commented Jan 17, 2025

Uh oh!

long76 commented Jan 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

long76 commented Dec 9, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 9, 2024 •

edited

Loading

long76 commented Dec 9, 2024 •

edited

Loading

panchenko commented Jan 14, 2025 •

edited

Loading